-
Notifications
You must be signed in to change notification settings - Fork 24.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[RCTImageLoader] Make C++ requirement opt-in #27730
[RCTImageLoader] Make C++ requirement opt-in #27730
Conversation
When building as a framework these headers get automatically added to the framework umbrella header for React-Core. Instead of converting all the React sources to ObjC++ files and still forcing external users that build native source (and link against a framework build) to also compile as ObjC++, this makes the attribution related methods opt-in to ObjC++ builds.
A back ported version for the 0.62 branch is here #27731 |
Looks like this is only a problem when running on iOS 13, CI runs iOS 12.4. Nonetheless a legit segfault, for when somebody on iOS 13 would try to serialise strings that can’t be utf8 encoded. Will do a bit of time-boxed sleuthing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@PeteTheHeat has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Hmm, I don't think I intended Maybe this import should do local header import instead:
|
Ah, I see. That should work, I’ll update it next week 👍 |
@fkgozali I took a stab at making these headers private, but found that–while it works well for framework builds–it will fail a static build. The reason is that e.g. RCTImageView.mm imports |
I see the complication now. RCTImageView.mm is pre-Fabric, so I'm hoping with Fabric rolled out, this gets easier to solve (because Fabric is ObjC++ everywhere). It seems like your current change is the most practical to unblock RC atm. I'll merge. |
This pull request was successfully merged by @alloy in 25571ec. When will my fix make it into a release? | Upcoming Releases |
Summary: When building as a framework these headers get automatically added to the framework umbrella header for React-Core. Instead of converting all the React sources to ObjC++ files and still forcing external users that build native source (and link against a framework build) to also compile as ObjC++, this makes the attribution related methods that were added in facebook@fdcdca4 opt-in to ObjC++ builds. This is also the reason for the current failure of the CI `test_ios_frameworks` run. I’m unsure if this change really warrants an entry in the CHANGELOG, as it’s more of an amendment of the (afaik) unreleased [change](facebook@fdcdca4). [iOS] [Fixed] - Make framework builds work again by making `RCTImageLoader` C++ requirement opt-in Pull Request resolved: facebook#27730 Test Plan: I tested static and dynamic (framework) builds and ran the test suite. This change should make the `test_ios_frameworks` CI run _build_ again, ~~but it may still fail overall as in my local testing one of the tests leads to a segfault (which I will try to address separately)~~. Reviewed By: PeteTheHeat Differential Revision: D19348846 Pulled By: fkgozali fbshipit-source-id: 8a74e6f7ad3ddce2cf10b080b9a5d7b399bd5fc0
Summary: When building as a framework these headers get automatically added to the framework umbrella header for React-Core. Instead of converting all the React sources to ObjC++ files and still forcing external users that build native source (and link against a framework build) to also compile as ObjC++, this makes the attribution related methods that were added in facebook@fdcdca4 opt-in to ObjC++ builds. This is also the reason for the current failure of the CI `test_ios_frameworks` run. ## Changelog I’m unsure if this change really warrants an entry in the CHANGELOG, as it’s more of an amendment of the (afaik) unreleased [change](facebook@fdcdca4). [iOS] [Fixed] - Make framework builds work again by making `RCTImageLoader` C++ requirement opt-in Pull Request resolved: facebook#27730 Test Plan: I tested static and dynamic (framework) builds and ran the test suite. This change should make the `test_ios_frameworks` CI run _build_ again, ~~but it may still fail overall as in my local testing one of the tests leads to a segfault (which I will try to address separately)~~. Reviewed By: PeteTheHeat Differential Revision: D19348846 Pulled By: fkgozali fbshipit-source-id: 8a74e6f7ad3ddce2cf10b080b9a5d7b399bd5fc0
Summary
When building as a framework these headers get automatically added to the framework umbrella header for React-Core. Instead of converting all the React sources to ObjC++ files and still forcing external users that build native source (and link against a framework build) to also compile as ObjC++, this makes the attribution related methods that were added in fdcdca4 opt-in to ObjC++ builds.
This is also the reason for the current failure of the CI
test_ios_frameworks
run.Changelog
I’m unsure if this change really warrants an entry in the CHANGELOG, as it’s more of an amendment of the (afaik) unreleased change.
[iOS] [Fixed] - Make framework builds work again by making
RCTImageLoader
C++ requirement opt-inTest Plan
I tested static and dynamic (framework) builds and ran the test suite.
This change should make the
test_ios_frameworks
CI run build again,but it may still fail overall as in my local testing one of the tests leads to a segfault (which I will try to address separately).